Skip to content

Fix #144: hermetic user_prompt_submit tests via TERRAPHIM_DEFAULT_DATA_PATH - #14

Merged
AlexMikhalev merged 1 commit into
mainfrom
task/144-hermetic-learning-capture
Sep 1, 2026
Merged

Fix #144: hermetic user_prompt_submit tests via TERRAPHIM_DEFAULT_DATA_PATH#14
AlexMikhalev merged 1 commit into
mainfrom
task/144-hermetic-learning-capture

Conversation

@AlexMikhalev

Copy link
Copy Markdown
Contributor

Fixes #144: user_prompt_submit_tests capture failures via
terraphim_agent::learnings, which historically writes correction
artefacts under dirs::data_dir(). On macOS that path resolves to
$HOME/Library/Application Support and ignores XDG_DATA_HOME,
so the tests could not redirect storage into a hermetic temp root.

Two coordinated changes:

  1. Production: LearningCaptureConfig::default() honours the
    TERRAPHIM_DEFAULT_DATA_PATH environment variable when computing
    its global_dir. This brings the hook path in line with the
    existing terraphim_settings::DeviceSettings, which already reads
    the same env var. storage_location() short-circuits to the
    global_dir when that variable is set, so the test never lands in
    the per-project directory by accident.

  2. Tests: each user_prompt_submit_tests case gets a unique hermetic
    root, derives its learnings_dir = root.join("data").join("terraphim").join("learnings"),
    spawns the agent with TERRAPHIM_DEFAULT_DATA_PATH pointing at
    the root's data dir, and reads back the correction files from
    that derived path. cli_test_env::create_hermetic_root() and
    set_hermetic_env() are new helpers in tests/support/cli_test_env.rs.

Local verification: cargo test -p terraphim_agent --test user_prompt_submit_tests reports 4 passed; 0 failed after this
change.

Copy link
Copy Markdown
Contributor Author

Summary

user_prompt_submit_tests in terraphim_agent had been failing on every non-Linux runner since the 2026-07-31 --lib-only gate regression. The root cause is platform-specific: LearningCaptureConfig::default() resolves global_dir via dirs::data_dir(), which on macOS and Windows returns $HOME/Library/Application Support and ignores XDG_DATA_HOME. The tests set HOME and XDG_DATA_HOME, but the production hook still wrote to the macOS data dir, so the assertions never found the expected files.

This PR makes two coordinated changes. In production:

  • LearningCaptureConfig::default() now honours TERRAPHIM_DEFAULT_DATA_PATH when computing global_dir, joining <root>/terraphim/learnings. This brings the hook path into line with the existing terraphim_settings::DeviceSettings, which already reads the same env var.
  • storage_location() short-circuits to global_dir whenever TERRAPHIM_DEFAULT_DATA_PATH is set, so the per-project .terraphim/learnings/ no longer wins in that case. When the env var is unset, behaviour is identical to before.

In tests:

  • support::cli_test_env::create_hermetic_root() and set_hermetic_env() are introduced. create_hermetic_root() returns the path of a fresh temp root so callers can read files written by the spawned subprocess; set_hermetic_env() points TERRAPHIM_DEFAULT_DATA_PATH at the root's data/ subdir.
  • All four tests (user_prompt_submit_use_instead_of_creates_tool_preference, user_prompt_submit_use_not_creates_tool_preference, user_prompt_submit_prefer_over_creates_tool_preference, user_prompt_submit_personal_preference_does_not_capture) now steer the spawned subprocess through set_hermetic_env and derive learnings_dir from the same root via a new hermetic_learnings_dir() helper. The pre-existing clear_correction_files() helper is removed (no longer needed -- each test gets a unique root).

What was done well: the production change is strictly additive (env var unset == previous behaviour), the rationale is laid out in the commit message with a clear before/after, no mocks or #[ignore] are introduced, and the test pattern matches what neighbouring test suites in this crate already use. All four tests pass locally; the personal-preference test, which was previously vacuous on macOS, now genuinely asserts that no file is created.

What remains problematic: the new tests/user_prompt_submit_tests.rs is missing the trailing newline at end of file (git shows \ No newline at end of file), which cargo fmt would otherwise add. There is also a small consistency gap: create_hermetic_root() creates root/data/ but the production code reads TERRAPHIM_DEFAULT_DATA_PATH and joins terraphim/learnings, so the storage location is root/data/terraphim/learnings while the hermetic root itself sits at root -- this is fine and well-commented, but the relationship between create_hermetic_root and the env-var contract could be tightened with one extra doc line.

Confidence Score: 4/5

  • Safe to merge with awareness of the missing trailing newline (P2) and the documentation-tightening suggestion (P2).
  • The production-side behaviour change is gated on an environment variable that is unlikely to be set by anyone except the test runner, so the risk to existing users is low. All four tests pass on macOS after the change.
  • No files require special attention beyond tests/user_prompt_submit_tests.rs.

Important Files Changed

Filename Overview
crates/terraphim_agent/src/learnings/mod.rs Default::default() honours TERRAPHIM_DEFAULT_DATA_PATH; storage_location() short-circuits to global_dir when the env var is set. Both paths documented inline. No issues found.
crates/terraphim_agent/tests/support/cli_test_env.rs Adds create_hermetic_root() and set_hermetic_env(); existing apply_hermetic_env() retained and refactored to call the new helpers. No issues found.
crates/terraphim_agent/tests/user_prompt_submit_tests.rs All four test cases rewritten to use the hermetic helpers. clear_correction_files() removed. Missing trailing newline at end of file.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[user_prompt_submit test] --> B[create_hermetic_root]
    B --> C["root: PathBuf"]
    A --> D["set_hermetic_env(cmd, &root)"]
    D --> E["cmd.env(TERRAPHIM_DEFAULT_DATA_PATH, root/data)"]
    A --> F[spawn terraphim-agent learn hook]
    F --> G["LearningCaptureConfig::default()"]
    G --> H{"TERRAPHIM_DEFAULT_DATA_PATH set?"}
    H -->|Yes| I["global_dir = root/data/terraphim/learnings"]
    H -->|No| J["global_dir = dirs::data_dir() ... (legacy)"]
    I --> K[storage_location returns global_dir]
    J --> L{"project_dir exists?"}
    L -->|Yes| M[storage_location returns project_dir]
    L -->|No| K
    K --> N[hook writes correction file]
    M --> N
    N --> O[test reads file via hermetic_learnings_dir root]
    O --> P[assert one file or zero files]

    style B fill:#d4edda,stroke:#28a745
    style D fill:#d4edda,stroke:#28a745
    style H fill:#fff3cd,stroke:#ffc107
    style I fill:#d4edda,stroke:#28a745
    style K fill:#d4edda,stroke:#28a745
Loading

Inline Findings

P2 crates/terraphim_agent/tests/user_prompt_submit_tests.rs, end of file: Missing trailing newline

git diff reports \ No newline at end of file for the new file. The rest of the workspace's Rust files are uniformly newline-terminated, and cargo fmt would re-add the newline on the next run. Trivial fix; just append one \n after the final }.

P2 crates/terraphim_agent/tests/support/cli_test_env.rs, lines 48-57 (create_hermetic_root): Contract between helper and env var is implicit

The helper creates root/data/ and set_hermetic_env points TERRAPHIM_DEFAULT_DATA_PATH at root/data/, while the production code joins <TERRAPHIM_DEFAULT_DATA_PATH>/terraphim/learnings. The two halves of the contract are correct but the link between create_hermetic_root's return value and the env-var value is implicit. A one-line doc comment on create_hermetic_root clarifying that callers should derive their expected-write directory as root.join("data").join("terraphim").join("learnings") (which is exactly what hermetic_learnings_dir does) would prevent the next reader from re-deriving the relationship. The same wording belongs on set_hermetic_env.

Last reviewed commit: e00c958 | Reviews (1)

…ULT_DATA_PATH (Refs #144)

The user-prompt-submit hook path uses LearningCaptureConfig::default() which
resolves global_dir via dirs::data_dir(). On macOS/Windows that ignores
XDG_DATA_HOME and returns $HOME/Library/Application Support, so the test
that set HOME and XDG_DATA_HOME never found the file it expected — 3 of 4
tests have been failing on every non-Linux runner since the --lib-only
regression of 2026-07-31.

Production:
- Honour TERRAPHIM_DEFAULT_DATA_PATH in Default::default() (matches the
  existing settings.toml field of the same name, bringing the hook path in
  line with every other terraphim-agent learn subcommand).
- storage_location() short-circuits to global_dir when TERRAPHIM_DEFAULT_DATA_PATH
  is set, so a project-local .terraphim/ directory no longer wins. This is a
  strict extension: when the env var is unset, behaviour is identical.

Test:
- Rewrite user_prompt_submit_tests to use support::cli_test_env::{create_hermetic_root,
  set_hermetic_env}. Each test gets a unique temp root; the spawned subprocess
  inherits TERRAPHIM_DEFAULT_DATA_PATH; the test reads back from the same
  hermetic root the hook wrote to.
- No mocks, no #[ignore], no timeout increases. The platform-specific
  dirs::data_dir() behaviour no longer matters.

All 4 tests pass (3 previously failing + 1 already passing, now non-vacuous
on macOS).
@AlexMikhalev
AlexMikhalev force-pushed the task/144-hermetic-learning-capture branch from e00c958 to 0258ba5 Compare September 1, 2026 00:22
@AlexMikhalev
AlexMikhalev merged commit 2ceda18 into main Sep 1, 2026
1 check failed
@AlexMikhalev
AlexMikhalev deleted the task/144-hermetic-learning-capture branch September 1, 2026 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant